-
Notifications
You must be signed in to change notification settings - Fork 3
Add Sites migration support #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds first-class Site migration support: new resource types Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 215-222: The call to Framework::STATIC() is invalid for Appwrite
v19.1.0 and will fatal; update the Sites creation in Appwrite.php by removing or
replacing Framework::STATIC() in the $this->sites->create(...) call inside the
Resource::TYPE_SITE block: either drop the framework argument if optional, or
map the incoming 'static' value to the correct Framework enum (e.g.,
Framework::REACTSTATIC or another valid Framework constant) before calling
$this->sites->create; update any other occurrences (e.g., the earlier match
handling that uses Framework::STATIC()) to perform the same mapping so the code
uses a valid Framework enum instead of Framework::STATIC().
In `@src/Migration/Transfer.php`:
- Around line 39-43: exportGroupSites currently only triggers exportSites() when
Resource::TYPE_SITE is requested, so requesting Resource::TYPE_SITE_VARIABLE
alone omits variables; update exportGroupSites() to also check for
Resource::TYPE_SITE_VARIABLE and either call exportSites() when variables are
requested or add a separate code path to export variables from exportSites()
output. Modify the conditional that tests for Resource::TYPE_SITE to include
Resource::TYPE_SITE_VARIABLE (or add an explicit branch handling
Resource::TYPE_SITE_VARIABLE) so site variables are exported even if TYPE_SITE
is not present, referencing the GROUP_SITES_RESOURCES constant and using
existing exportSites() logic to perform the actual extraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1542-1635: The code uses Appwrite enums (Framework, Adapter,
BuildRuntime) that may not exist in the installed SDK and will throw fatal
errors; replace enum usage in the match blocks (the mappings that assign
$framework, $adapter, $buildRuntime from
$resource->getFramework()/getAdapter()/getBuildRuntime()) with plain string
values (or a validated string mapping) instead of Framework::*, Adapter::*,
BuildRuntime::* calls, or alternatively add a runtime guard (class_exists for
each enum) and fallback to the raw string when the enum class is missing; update
the match blocks in src/Migration/Destinations/Appwrite.php (the $framework,
$adapter, $buildRuntime assignments) to return strings (e.g., 'react', 'static',
'node-18.0') or use a safe mapping and remove direct enum references unless the
SDK dependency is constrained to a release that includes those enum classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1533-1669: The importSiteResource method may try to create
SiteVariable or SiteDeployment before their parent Site exists because calls
like $resource->getSite()->getId() (used in sites->createVariable) and
importSiteDeployment($resource) assume the Site was already created; fix by
ensuring parent Site exists before handling Resource::TYPE_SITE_VARIABLE and
Resource::TYPE_SITE_DEPLOYMENT — either enforce import ordering upstream or add
a guard in importSiteResource: check that $this->sites has the parent site (or
that $resource->getSite()->getId() resolves to a created site) and if missing,
queue the variable/deployment for retry or throw a clear, recoverable exception
so the caller can import the parent first (apply this check around the
SiteVariable handling block and before calling importSiteDeployment).
- Around line 1675-1732: The Content-Range header in importSiteDeployment is
incorrectly computing the final chunk end as getSize() instead of the inclusive
byte offset getSize() - 1; update the header construction in the client->call
('content-range' value) to use ($deployment->getEnd() == ($deployment->getSize()
- 1) ? $deployment->getSize() - 1 : $deployment->getEnd()) so the final chunk is
"bytes start-(size-1)/size" per RFC 7233; locate this change in
importSiteDeployment where the client->call headers are built (look for
'content-range' and methods getStart(), getEnd(), getSize()).
- Around line 216-223: Replace the empty strings passed to the Appwrite probe so
the SDK validation is exercised: in the block that checks Resource::TYPE_SITE,
update the call to $this->sites->create('', '', Framework::OTHER(),
BuildRuntime::STATIC1()) to use non-empty placeholder values for siteId and name
(e.g. 'test', 'test') so Sites::create() receives realistic parameters and
avoids potential SDK validation errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1849-1933: In exportSiteDeploymentData() the use of mb_strlen() to
determine binary download size can miscount bytes; replace mb_strlen($file) with
strlen($file) (and cast/ensure it is an int) so fileSize/size calculations and
range handling are byte-accurate for small non-chunked downloads; update the
variable set at the non-chunk branch and any comparisons that depend on it
(e.g., the $end > $size check) to use the new byte-accurate value.
- Around line 1479-1508: The exportGroupSites method currently only calls
exportSites when Resource::TYPE_SITE is requested, so requests that include
Resource::TYPE_SITE_VARIABLE but not TYPE_SITE skip exporting sites and their
variables; update the condition in exportGroupSites to call
$this->exportSites($batchSize) when either Resource::TYPE_SITE or
Resource::TYPE_SITE_VARIABLE is present, and ensure exportSites/export logic
(where site variables are collected) validates that a parent site exists before
exporting each TYPE_SITE_VARIABLE (so variables are never exported in
isolation).
Summary
Changes
Sites/Site.php,Sites/Deployment.php,Sites/EnvVar.phpexportGroupSites,exportSites,exportSiteDeployments,exportSiteDeploymentData,reportSitesinSources/Appwrite.phpimportSiteResource,importSiteDeploymentwith Framework/BuildRuntime/Adapter enum mappings inDestinations/Appwrite.phpTYPE_SITE_DEPLOYMENThandling for cache key resolution and memory leak preventionGROUP_SITESconstant andGROUP_SITES_RESOURCESarrayTYPE_SITE,TYPE_SITE_DEPLOYMENT,TYPE_SITE_VARIABLEconstantsTest plan
Summary by CodeRabbit
New Features